Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed redundant database selection query #34389

Closed
wants to merge 1 commit into from
Closed

Removed redundant database selection query #34389

wants to merge 1 commit into from

Conversation

chrispage1
Copy link
Contributor

Hello!

Due to an issue I was having with connecting Laravel to MariaDB's Maxscale, I came across these lines of redundant code. Ironically, they were the actual lines triggering the issue I was having.

Lines 26-28 in Illuminate\Database\Connectors\MySQLConnector perform a query to use the given database name, based on the MySQL Database connection config having a database flag.

However, due to how PDO establishes connections, the database is already being chosen within the DSN generation that's being performed, on line 113 and called within the connect method on line 17.

This makes the use call being made redundant and can be removed.

I've tested this change on MySQL 5.7, MySQL 8.0 & MariaDB 10.1 & 10.5 and everything works as expected.

@taylorotwell
Copy link
Member

Feels scary to remove something that's been there so long on a patch release. Sure every edge case has been thought of?

@chrispage1
Copy link
Contributor Author

chrispage1 commented Sep 17, 2020

Agreed - although I can't see any need for it at all. PDO had already established a connection with the database in question. I presume if it's a multi database setup it'd re-establish the controller completely? Also whenever this would be called the DSN would have also been initiated.

With Laravel requiring >= PHP 7.3 there aren't any backwards incompatibilities with PDO/MySQL.

Comment on lines -26 to -29
if (! empty($config['database'])) {
$connection->exec("use `{$config['database']}`;");
}

Copy link
Contributor

@mfn mfn Sep 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the initial implementation I dug out, there is a comment which has been removed over time (did not check why), but I guess it makes sense to check the mentioned case before fiddling around here => c203b02#diff-907ccb2ad354af6868ebb5618f2da86bR22-R23

See also #4138

@chrispage1
Copy link
Contributor Author

Great - thanks @mfn - I'll take a look tomorrow and test with sockets - a lot has changed in six years! 😅

@taylorotwell
Copy link
Member

I'm not sure we should be tinkering around with this on a patch release.

@chrispage1
Copy link
Contributor Author

Thanks, Taylor, understood. I've written a package that makes this change and can be pulled in. As for me, the use statement with escaped backticks was killing my connection to MariaDB MaxScale. For anyone it may be of use to - https://packagist.org/packages/motomedialab/maxscale-connector

@RickJeroen
Copy link

MariaDB MaxScale will fix this in the next 2.5.6 release. See: https://jira.mariadb.org/browse/MXS-3292

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants